Maybe use https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/reduce instead. inline in https://github.com/wxt-dev/wxt/pull/2288/changes#diff-50df625c6f95cac7dcf47882681f0bc55fb27a66858fc50256f26fb541b25c43R450

I’m not sure reduce makes sense in addOptionalPermission, since that helper is just ensuring the array exists, deduping, and mutating the manifest. Did you mean refactoring the optionalContentScripts.forEach(...) logic, rather than the manifest.optional_permissions.push(permission) part?

Yeah i mean refactor optionalContentScripts.forEach(...) because you have nested forEach and it doesn't look good.

I think you can omit entire addOptionalPermission, but now i have better idea.

Let's use .reduce like it's in line 414 for hostPermissions, it'll be good enough for there.

I should add this comment above, but as i mentioned here, i want to do it in other way previously 😆

Could you suggest the change please? I can't think of how using Array.prototype.reduce can help with readability. The optionalContentScript is pretty much identical to the existing code:

  // Runtime content scripts
  const runtimeContentScripts = contentScripts.filter(
    (cs) => cs.options.registration === 'runtime',
  );
  runtimeContentScripts.forEach((script) => {
    script.options.matches?.forEach((matchPattern) => {
      addHostPermission(manifest, matchPattern);
    });
  });

Yeah, I'm not sure what you're requesting here either @PatrykKuniczak

Ok guys, my bad .reduce() here, isn't good.
But IMO a little better is:

contentScripts
  .filter((cs) => cs.options.registration === 'optional')
  .flatMap(script => script.options.matches || [])
  .forEach((matchPattern) => {
    addOptionalHostPermission(manifest, matchPattern);
  });

But if you think it isn't let's leave what it is.

The same like for addOptionalPermission

What if i want to use both in the same time?

I think that’s a totally fair point, and I hadn’t considered that case.

export default defineContentScript({
    matches: ['https://my-app.com/*'],
    registration: 'optional',
    // ...
});

The original issue’s proposed API treats registration as a single option for the whole content script, so it doesn’t currently allow choosing required vs optional behavior per match.

If we want to support both without introducing breaking changes, maybe we could extend the API, for example by adding a new optional field to separate optional matches from required ones. That way the current API keeps working, while still leaving room for mixed-permission content scripts.

Maybe something like this..

export default defineContentScript({
  matches: ['https://required.com/*'],
  optionalMatches: ['https://optional.com/*'],
})

export default defineContentScript({
  matches: ['https://required.com/*'],
  optionalMatches: ['https://optional.com/*'],
})

Yeah i was thinking about that, because we can't close our users to THIS or THIS approach, especially if on native approach, you can have both.

@marcellino-ornelas @aklinker1 Any feedback on this approach?

Hmm... I like optionalMatches. As I was reviewing this, I was starting to get confused how we would describe the difference between runtime and optional, having a separate property might make more sense.

Nice catch on this @PatrykKuniczak

I like the optionalMatches here as well 🙌🏽 This is actually something were supporting right now in our custom module wasnt sure how to express this at the time of my issue tho soo left this out. Being able to support both like this is amazing and solves all of our usecases ❤️

although how would the registration option work here now? you need registration: 'runtime' soo it doesn't hit the manifest and optional content scripts need the registration: 'runtime' but your matches may want to use registration: 'manifest'

Unless were saying here all matches goes to manifest and all optionalMatches goes to runtime. The only weird case would be if you wanted your matches configured to runtime and have them go to host_permissions which I haven't found a use case for that yet but maybe someone out there does?

This doesn't go here. Let's put it here instead: https://wxt.dev/guide/essentials/content-scripts.html

We should add an entire section that covers all three options.

I need to update the example from above to not use defineContentScript, you should use an unlisted script for that use-case.

Hmm... I like optionalMatches. As I was reviewing this, I was starting to get confused how we would describe the difference between runtime and optional, having a separate property might make more sense.

Yeah, I'm not sure what you're requesting here either @PatrykKuniczak

Left this comment separately but you might want to consider using MatchPatterns here soo not only do you check whether its included but you can also check to see if a more wildcard host captures this host more broadly

see this comment for more details:
#2288 (comment)

Love the deduping idea, and thanks for the implementation tips!

I am considering using @webext-core/match-patterns as it's already in wxt core dependency. I just need some feedback on the pattern matching API : #2288 (comment)